Add initial Bitrefill widget to staging #3343
Conversation
There was a problem hiding this comment.
Very nice LGTM, added some small comments
also @Beerosagos please review if you have a moment.
| @@ -21,7 +21,7 @@ export const getExchangeRegionCodes = (): Promise<string[]> => { | |||
| return apiGet('exchange/region-codes'); | |||
| }; | |||
|
|
|||
| export type TPaymentMethod = 'card' | 'bank-transfer' | 'bancontact' | 'sofort'; | |||
| export type TPaymentMethod = 'card' | 'bank-transfer' | 'bancontact' | 'sofort' | 'spend'; | |||
There was a problem hiding this comment.
nit: 'spend' looks a bit wrong here, but I haven't yet checked why it is needed…
There was a problem hiding this comment.
Yeah it's a bit ugly. I'm basically abusing the payment method field to pass a custom message (which depends on the provider). Strictly speaking "spend" is the payment method here, i.e. the wallet itself pays.
sutterseba
force-pushed
the
staging-bitrefill
branch
2 times, most recently
from
a90ae85 to
4440b2a
Compare
Integrates Bitrefill widget into the backend and adds a new "spend" action, as Bitrefill is a new type of exchange next to buy and sell.
Integrates Bitrefill in the frontend. On the previous "Buy & sell" menu, there is now a third selectable pill "Spend", opening the available exchange options (Bitrefill in this case). The widget is requested from bitboxapp.shiftcrypto.io using config parameters from the user/app, handled in the "request-configuration" event. Note that the iframe will still have its source set to "https://embed.bitrefill.com". When the user clicks "Pay" inside the Bitrefill widget to purchase a gift card, the returned address and amount is used to propose a transaction on the connected BitBox. The PaymentMethod component is adjusted to allow custom messages to be shown in the exchange overview, as these will differ in the "Spend" section between providers. Disclaimer and info texts are added technically, but the actual text is still missing in this commit. Bitrefill developer documentation: https://www.bitrefill.com/playground/documentation/url-params
sutterseba
force-pushed
the
staging-bitrefill
branch
from
57d1a0b to
b26e651
Compare
|
@thisconnect addressed comments and also changed to the same tx note format now (as in #3351) |
There was a problem hiding this comment.
very nice, initial Bitrefill pr LGTM
Lets get this merged to staging-bitrefill soon, you've already over-delivered and it looks like almost done and more than just a POC :)
ping @Beerosagos ptal
Uses the same "Bitrefill payment XYZ" format for transaction notes of Bitrefill spends, which other integrations use as well.
sutterseba
force-pushed
the
staging-bitrefill
branch
from
c1fe7f1 to
0dd92bc
Compare
I already saw the first mention 😂 I'll have a look asap |
I didn't want to rush you, but wanted to hand it over to you. 😇 |
| paymentRequest: null | ||
| }; | ||
|
|
||
| let result = await proposeTx(code, txInput); |
There was a problem hiding this comment.
tiny nit: by using const it would tell that this variable will not be re-assigned later.
| return; | ||
| } | ||
|
|
||
| const data = typeof event.data === 'string' ? JSON.parse(event.data) : event.data; |
There was a problem hiding this comment.
JSON.parse could throw an error if event.data is not valid JSON, this probably never happens, however it is coming from outside the BitBoxApp so it would not be wrong to wrap in try/catch
|
I'm not super convinced about how the backend has been implemented. Using the exchange endpoints for Bitrefill (e.g. adding it to the exchange deals, where both |
There was a problem hiding this comment.
I tested some changes in the code and I realized that the Deal model is deeply exploited in the FE implementation and that makes quite hard to move Bitrefill outside of it in a nice way. It probably doesn't make much sense from the effort pov.
I think that we could merge this and refactor later some part of the model, instead, to make it more flexible (e.g. not only valid for exchanges) so that it can fit Bitrefill and other future integrations.
LGTM, great work @sutterseba 🔥
Adds an initial proof of concept integration of the Bitrefill widget.
Still missing: